-
Notifications
You must be signed in to change notification settings - Fork 665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change: api revisions for zero-alloc sendmmsg and recvmmsg #2459
Conversation
Thanks for the kind words, though I would say the credit goes to asomers. The issue described here does exist, it enforces user to heap-allocate the Thanks for that demo repo! BTW.
And I agree that the reference in the signature is unnecessary considering we already borrow the buffer through
#2457 is different from this,
Yeah, it depends on the usage. We will have breaking changes in the next release, so no need to worry about it.
Did you see this in our CI? I don't remember I have seen it before:< |
Appreciate it! Sounds great. |
Hey! Really sorry, I didn't realize until after trying to use these changes and seeing unexpected output, but I think removing the borrow here might actually cause Undefined Behavior. My new understanding is below. I'm closing the PR to prevent merge. If what I have below is wrong, please let me know and I'll re-open it: Anyway, here's the function signature AND the first few lines of pub fn sendmmsg<'a, XS, AS, C, I, S>(
fd: RawFd,
data: &'a mut MultiHeaders<S>,
slices: XS,
// one address per group of slices
addrs: AS,
// shared across all the messages
cmsgs: C,
flags: MsgFlags
) -> crate::Result<MultiResults<'a, S>>
where
XS: IntoIterator<Item = I>,
AS: AsRef<[Option<S>]>,
I: AsRef<[IoSlice<'a>]> + 'a,
C: AsRef<[ControlMessage<'a>]> + 'a,
S: SockaddrLike + 'a,
{
...
for (i, ((slice, addr), mmsghdr)) in slices.into_iter().zip(addrs.as_ref()).zip(data.items.iter_mut()).enumerate() {
let p = &mut mmsghdr.msg_hdr;
p.msg_iov = slice.as_ref().as_ptr().cast_mut().cast();
p.msg_iovlen = slice.as_ref().len() as _; In my example from the initial summary, the iterator yields this: I was frustrated that this function requires the IoSlices to be in a borrowed collection. However, I think it was written that way because the msghdr struct takes a pointer to an array of iovecs. A zero-alloc *mmsg scheme for this in Rust still feels possible, but I don't think it'll be nearly as trivial as I expected. |
Huh, thanks for catching it! That is indeed a dangling pointer, and since we are using raw pointer here, it wasn't caught by the borrow checker. And taking a closer look at the Thanks again for catching it before I merge the PR! |
Change *mmsg to support memory reuse
In short, these changes adjust the generics on
sendmmsg
andrecvmmsg
so that both can be used in a loop without requiring new heap allocations from the caller on each iteration.Motivation
First, thanks much to the maintainers of this library. It's very solid, and I use it heavily. However, with some frustration, I've been unable to find a way to use
sendmmsg
andrecvmmsg
in a loop without making any heap allocations. If someone knows how to with the current API, please just show me a compiling example and I'll close this PR. Otherwise, here's the issue as I understand it. I'll usesendmmsg
for analysis, but both have the same issue:Current:
Proposed:
sendmmsg
requires its buffers to be wrapped in anIoSlice
. BecauseIoSlice
takes the lifetime of the buffer it wraps, the underlying buffer cannot be mutated until theIoSlice
is dropped. So, if the buffer allocation is reused between calls tosendmmsg
, then theIoSlice
needs to be dropped. If there's a non-constant number ofIoSlices
, then they need to be in an iterator or some dynamic collection.Because
I
is borrowed in the signature, theIoSlice
cannot be produced by an iterator because a reference cannot be returned to anIoSlice
created within the same iterator. That basically mandates that theIoSlice
instances are collected into aVec
beforesendmmsg
is called.However, I don't think a reference is actually needed here. Making
I
an&'a AsRef<_>
is redundant because we useAsRef
to borrow the data anyway. My changes remove the borrow overI
fromsendmmsg
andrecvmmsg
. This allows theIoSlice
andIoSliceMut
instances to be yielded from an iterator instead of a heap-allocated collection, which gives a meaningful performance boost to my code using these functions.Also, I noticed someone else recently made an issue related to this. Perhaps this would satisfy them as well? #2457
Validation
I made a small repo to demo my use case and how these changes help. It can be found here: https://github.com/cfzimmerman/nix-alloc-demo/blob/main/src/lib.rs
The important part is this comparison in extension of the example above.
Using the current API:
Using the proposed API:
I do not know if this is a breaking API change. From the limited examples I've tried, it doesn't seem to be. However, I can't guarantee that and would appreciate input from someone with more experience.
Also, I encountered a test that seems flaky, but maybe there are issues on my side. Both before and after my changes, the test
test_recvmm2
failed for me occasionally. Is this expected? Here's a demo of failure before I made any changes. If this is a reason not to merge, please let me know and I'll try to debug first.Thanks for considering these changes!
Checklist:
CONTRIBUTING.md